fix: rework user group management with an accessible selector#1090
Merged
igboyes merged 3 commits intoJul 2, 2026
Merged
Conversation
Replace the toggle-checkbox group list and the separate primary-group dropdown with a single accessible control: a combobox that adds groups to the membership list, and a radio group where each member row selects the primary group and offers a button to revoke membership. The old list rendered each row as role="option" wrapping a Radix checkbox, producing conflicting roles, double tab stops, and a dead decorative checkbox. It also scaled poorly as the number of groups grew. Generalize the orphaned base ComboBox with a placeholder prop and match its height to the standard Input. Remove UserGroup and PrimaryGroup.
The add-group combobox previously showed an empty search box both when a user already belonged to every group and when no groups existed at all — two very different situations that looked identical. Distinguish them: show "This user is a member of every group" when there is nothing left to add, and a "No groups have been created yet" message linking to group management when the instance has no groups.
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- Consider memoizing
memberIdsandavailableGroups(e.g. withuseMemo) so they aren’t recomputed and reallocated on every render, especially since this component is intended to scale to large group counts. - In the
useListGroupserror case you currently render<QueryError />only fromrenderAdd, whilerenderMembershipstill runs; you may want to short-circuit the whole component on a fatal groups error to avoid partially-rendered or confusing UI states.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider memoizing `memberIds` and `availableGroups` (e.g. with `useMemo`) so they aren’t recomputed and reallocated on every render, especially since this component is intended to scale to large group counts.
- In the `useListGroups` error case you currently render `<QueryError />` only from `renderAdd`, while `renderMembership` still runs; you may want to short-circuit the whole component on a fatal groups error to avoid partially-rendered or confusing UI states.
## Individual Comments
### Comment 1
<location path="apps/web/src/users/components/UserGroups.tsx" line_range="184-185" />
<code_context>
+ }
+
+ return (
+ <div className="mb-4">
+ <InputLabel>Groups</InputLabel>
+ {renderAdd()}
+ {renderMembership()}
</code_context>
<issue_to_address>
**issue (bug_risk):** The label is no longer programmatically associated with the interactive controls, which is an accessibility regression.
Previously this used `useId` and `aria-labelledby` on `BoxGroup` to tie the label to the listbox. Now `InputLabel` has no `id`/`htmlFor`, and neither the combobox nor radio group reference it. Please either restore an `id` on `InputLabel` and point the combo/radio group to it via `aria-labelledby`, or wire `InputLabel`’s `htmlFor` to the appropriate control.
</issue_to_address>
### Comment 2
<location path="apps/web/src/users/components/__tests__/UserGroups.test.tsx" line_range="46-23" />
<code_context>
+ ).not.toBeChecked();
+ });
+
+ it("shows an empty message when the user has no groups", async () => {
+ mockApiListGroups(allGroups);
+
+ renderWithProviders(
+ <UserGroups userId={userId} memberGroups={[]} primaryGroup={null} />,
+ );
+
+ expect(
+ await screen.findByText("This user is not a member of any groups."),
+ ).toBeInTheDocument();
+ });
+
+ it("hides the combobox when the user is in every group", async () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for the case where a user has groups but no primary group.
The remaining branch is when `memberGroups` is non-empty but `primaryGroup` is `null`, where the "No primary group" radio should be selected by default. Please add a focused test for this combination to cover that default-selection behavior.
</issue_to_address>
### Comment 3
<location path="apps/web/src/users/components/__tests__/UserGroups.test.tsx" line_range="121-23" />
<code_context>
+ );
+ });
+
+ it("sets the primary group when a radio is selected", async () => {
+ mockApiListGroups(allGroups);
+ mockApiEditUser(userId, 200, {});
+
+ renderWithProviders(
+ <UserGroups
+ userId={userId}
+ memberGroups={[member, other]}
+ primaryGroup={member}
+ />,
+ );
+
+ await userEvent.click(await screen.findByRole("radio", { name: "bar" }));
+
+ await waitFor(() =>
+ expect(userServerFnMocks.updateUser).toHaveBeenCalledWith({
+ data: { userId, primary_group: 2 },
+ }),
+ );
+ });
+
+ it("removes a group and clears the primary when the primary is removed", async () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Also test selecting the "No primary group" radio to clear `primary_group`.
To fully cover the new behavior, please add a test that clicks the "No primary group" radio and asserts `updateUser` is called with `primary_group: null`, so the transition from having a primary group to none is also verified.
Suggested implementation:
```typescript
it("sets the primary group when a radio is selected", async () => {
mockApiListGroups(allGroups);
mockApiEditUser(userId, 200, {});
renderWithProviders(
<UserGroups
userId={userId}
memberGroups={[member, other]}
primaryGroup={member}
/>,
);
await userEvent.click(await screen.findByRole("radio", { name: "bar" }));
await waitFor(() =>
expect(userServerFnMocks.updateUser).toHaveBeenCalledWith({
data: { userId, primary_group: 2 },
}),
);
});
it("clears the primary group when selecting the 'No primary group' option", async () => {
mockApiListGroups(allGroups);
mockApiEditUser(userId, 200, {});
renderWithProviders(
<UserGroups
userId={userId}
memberGroups={[member, other]}
primaryGroup={member}
/>,
);
await userEvent.click(
await screen.findByRole("radio", {
name: /no primary group/i,
}),
);
await waitFor(() =>
expect(userServerFnMocks.updateUser).toHaveBeenCalledWith({
data: { userId, primary_group: null },
}),
);
});
it("removes a group and clears the primary when the primary is removed", async () => {
```
This assumes the "No primary group" radio is rendered with an accessible name matching `/no primary group/i`. If the actual label differs, adjust the `name` matcher in `findByRole` accordingly to match the existing UI text.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Render the "Groups" heading as a span instead of a label with no associated control. Each control keeps its own accessible name (the combobox "Add group", the radio group "Primary group"). Add tests for the default "No primary group" selection and for clearing the primary group.
Member
Author
|
Re the two review-body suggestions — both intentionally skipped:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
option/checkboxroles, double tab stops, dead decorative checkbox) and scales better as group counts grow.ComboBoxwith aplaceholderprop and match its height to the standardInput; removeUserGroupandPrimaryGroup.